It seems somewhat problematic to me to discard all information about the signature algorithm.

Based on the TryFrom impl below it looks like this would accept any bytestring as a "signature" except for empty ones, and only tries to validate that the signature is well-formed for a particular algorithm lazily at verification time, which means this type could accept all sorts of potential garbage data as a signature.

This includes signatures that are not the valid size for a particular algorithm, or signatures whose elements are't a valid element of e.g. the scalar field for a particular elliptic curve.

Yeah we tried finding another way around this, but there's no discriminant built into the signatures themselves that allows you to determine what kind of signature it is.

How else would one know this up-front, so that you could validate them beforehand?

The only way I can think of would require changing our serialization approach substantially, such that we can only deserialize signatures from within data structures that provide information about what algorithm was used. This complicates the serialization picture significantly.

I was wondering if there was a multihash (https://github.com/multiformats/multihash) equivalent for signatures.
I haven't been able to find one but it would be nice to have some sort of standardized self-describing signatures.

Self-describing signatures would be amazing, but that change would need to happen in the Tendermint RPC.

At the very least the only two supported signature types are both 64-bytes, so you could check for that.

It would probably be a good idea to eventually fix this upstream in Tendermint, but if it doesn't do that already I agree there's nothing relevant to this PR.

I agree, a 64-byte length check would be good 👍

try using k256::ecdsa::Signature::from_scalars to create signature

pub fn new_secp256k1_signature(value : &[u8]) -> Result<Secp256k1Signature, Error> {
        if value.len() != 64 {
            return Err(Kind::InvalidSignatureIdLength.into());
        }
        let (r_bytes, s_bytes) = value.split_at(32);

        let r = match NonZeroScalar::from_repr(GenericArray::clone_from_slice(r_bytes)) {
            Some(v) => v,
            None => return Err(Kind::InvalidSignature.into()),
        };

        let s = match NonZeroScalar::from_repr(GenericArray::clone_from_slice(s_bytes)) {
            Some(v) => v,
            None => return Err(Kind::InvalidSignature.into()),
        };

        Secp256k1Signature::from_scalars(r,s).map_err(|_| Kind::InvalidSignature.into())
    }

TryFrom<&[u8]> will also accomplish the same thing, and is a bit simpler, e.g.

Secp256k1Signature::try_from(value).map_err(|_| Kind::InvalidSignature.into())

It might be good to add some test vectors for both of these, just to ensure that a valid signature verifies and an invalid signature is rejected.

I can get some if you'd like.

Here are the RFC 8032 test vectors for Ed25519:

https://datatracker.ietf.org/doc/html/rfc8032#section-7.1

Here is an ECDSA/secp256k1 test vector:

https://github.com/RustCrypto/elliptic-curves/blob/master/k256/src/test_vectors/ecdsa.rs

There are quite a few ECDSA/secp256k1 test vectors in the Wycheproof test suite if you want more.

I've tried adding a few test vectors from RFC 8032 and the Wycheproof test suite in #964.

This is somewhat orthogonal to the changes in this PR, but I'm curious why this is TryFrom<Vec<u8>> instead of TryFrom<&[u8]>. The latter is a lot more flexible since you can use it with anything which can be borrowed as a byte slice (e.g. Bytes)

The TryFrom<Vec<u8>> instance is required by the Protobuf<Vec<u8>> instance, but I agree that we should add a TryFrom<&[u8]> instance as well.

pub trait Protobuf<T: Message + From<Self> + Default>
where
    Self: Sized + Clone + TryFrom<T>, // <---------
    <Self as TryFrom<T>>::Error: Display,